Skip to content

plugins/hooks: run hooks when exit code != 0 - #5024

Merged
laurazard merged 1 commit into
docker:masterfrom
laurazard:run-hooks-error
Apr 18, 2024
Merged

plugins/hooks: run hooks when exit code != 0#5024
laurazard merged 1 commit into
docker:masterfrom
laurazard:run-hooks-error

Conversation

@laurazard

Copy link
Copy Markdown
Member

- What I did

This PR changes the hooks flow so that hooks are still invoked after the command execution returned an error/non-zero exit code.

I ran into this with docker exec -it [] sh, since I was exiting with CTRL+D after doing things inside a container and depending on the exit code my hooks wouldn't get invoked, which isn't desired.

In the future, this might also be interesting to allow plugins to run hooks after an error so they can offer error-state recovery suggestions, although this would require additional work to give the plugin more information about the failed execution.

- How to verify it

Try hooks after a non-zero exit code docker exec

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

image

Particularly for cases such as `docker exec -it`, it's relevant that the CLI
still executes hooks even if the exec exited with a non-zero exit code,
since this is can be part of a normal `docker exec` invocation depending on
how the user exits.

In the future, this might also be interesting to allow plugins to run
hooks after an error so they can offer error-state recovery suggestions,
although this would require additional work to give the plugin more
information about the failed execution.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@laurazard laurazard added this to the 26.1.0 milestone Apr 17, 2024
@laurazard laurazard self-assigned this Apr 17, 2024
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #5024 (c449c1a) into master (c0cc22d) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5024   +/-   ##
=======================================
  Coverage   61.03%   61.04%           
=======================================
  Files         295      295           
  Lines       20648    20646    -2     
=======================================
  Hits        12603    12603           
+ Misses       7148     7146    -2     
  Partials      897      897           

@krissetto krissetto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@laurazard
laurazard merged commit 78089c5 into docker:master Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants